Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: member 상태관리를 위한 member.status 추가에 따른 수정 #200

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

IW-MOON
Copy link
Collaborator

@IW-MOON IW-MOON commented Apr 2, 2023

📌Linked Issues

✏Change Details

  • status 추가에 따른 코드, flyway ddl, 테스트코드 수정

💬Comment

  • 활동회원 : NULL, 탈퇴회원 : 1로 변경하도록 하였습니다. 후에 더 좋은 관리방법을 생각해볼 필요는 있겠네요~

📑References

✅Check List

  • [✅] 추가한 기능에 대한 테스트는 모두 완료하셨나요?
  • [✅] 코드 정렬(Ctrl + Alt + L), 불필요한 코드나 오타는 없는지 확인하셨나요?

@IW-MOON IW-MOON added enhancement New feature or request feature 새로운 기능 개발 labels Apr 2, 2023
@@ -39,6 +39,9 @@ public class Member extends BaseTimeEntity {
@Column(nullable = true)
private String job;

@Column(nullable = true, length = 1)
private Integer status;
Copy link
Member

@junhaesung junhaesung Apr 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 일 때 탈퇴, null 일 때 일반 회원이라는 의미로 정한다면 우려되는점이 몇 가지 있는데요.

우선 1이라는 magic number 가 탈퇴 회원인 상태라는 걸 나타낸다고 알고있어야 코드를 이해할 수 있어서 가독성면에서 단점이 있는것같습니다. 상태가 늘어난다면 2,3,4 마다 각각 다른 의미를 가질텐데, 숫자와 의미를 매핑해야해서요.

그리고 nullable 한 타입을 사용할 때 null에 다른 의미를 부여하게되면, 실수로 null 을 입력한것과 제대로 null 을 입력한 것을 구분하기 어려워서 이슈가 생겼을 때 데이터를 추적하기에 어려울 것 같습니다.

MemberStatus 등의 enum 을 만들고, ACTIVE, WITHDRAWAL 등의 상수로 회원의 상태를 구분하는건 어떠쎄요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음에 생각했던 방법인데요~
상수로 한다면, 컬럼 속성 및 데이터 변경 등의 DDL 작업과 merge 작업을 최대한 맞춰서 해야할 것 같고요~
enum converter 사용도 한번 고려해보면 좋을 것 같아요~

장단점이 있을 것 같은데~ 현재 구조를 유지하면서 간단하게 변경할 수 있는 것으로 우선 적용해 놓은 것입니다~

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

새 컬럼을 추가하는거라면 코드 머지 시점과 맞출 필요는 없고, 배포 이전에만 DDL 먼저 적용해도됩니다.
컬럼 데이터 migration 이 필요한데요.
추가된 컬럼의 null 을 수정하기 전에, null 도 일반 회원이라는 등의 로직을 작성하기 위해 AttributeConverter 말씀하신거라면, 굳이 필요하지 않을 것 같습니다.
꼭 무중단으로 운영할필요도 없고, 데이터도 많지않으니새 코드 적용하기전에 서버 잠깐 내리고 db 작업하면 두 번 배포하지 않아도 되어서요.

enum 대비 null 로 처리해서 얻을 수 있는 코드작성시점의 시간적 이득보다, 앞으로 member 코드를 읽으면서 의문점이 남거나 불편한 단점이 더 크다고 생각해서 enum 으로 처리하면 좋을 것 같습니다-

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 converter 사용을 고려한 이유는 다음글들을 참고해서 db 공간 낭비를 생각해서였습니다.
https://pamyferret.tistory.com/6
https://nomadlee.com/mysql-%EC%B5%9C%EC%A0%81%EC%9D%98-%EB%8D%B0%EC%9D%B4%ED%84%B0-%ED%83%80%EC%9E%85-%EC%84%A0%ED%83%9D-%EB%B0%A9%EB%B2%95/

말씀하신 대로 서비스 규모가 작으니.. 이런 것들을 고려할 필요할 있을가? 라는 생각도 하게 됐고..
그러다 보니 다시 enum 사용 없이 제일 단순한 구조로 오게 되었습니다.

Copy link
Member

@junhaesung junhaesung Apr 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

서비스 규모가 작으니 바이트 단위로 용량 아끼는건 생략해도 좋겠다는 의견에 동의하고요.
다만 null 에 의미 부여하는 코드는 가독성면에서 받아들이기 어렵네요

enum 을 사용할때 숫자로 저장할지 문자로 저장할지는 크게 중요한 이슈 아닌거같습니다.
다른사람이 코드를 읽었을 때 '왜 null 정상인 멤버이지?'라고 고민하게되는 비용이 제일 크게 작용할거같아요

migration 및 코드 수정하는 비용은 1시간 미만일거같은데, 앞으로 코드 읽으면서 헷갈린다면 그렇게 낭비되는 시간이 더 오래걸릴 것 같습니다.

@@ -0,0 +1,2 @@
alter table member
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flyway 거의 신경 못쓰고있었는데 감사합니다 👍

@IW-MOON IW-MOON force-pushed the feature/196-add-member-status branch from b530e5f to 75758fa Compare May 7, 2023 14:55
@IW-MOON IW-MOON force-pushed the feature/196-add-member-status branch from 75758fa to 2fef692 Compare May 7, 2023 15:03
@sonarcloud
Copy link

sonarcloud bot commented May 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

8.3% 8.3% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature 새로운 기능 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants